[WIP] - Add viewer.py to Habitat-sim module #1990

aclegg3 · 2023-01-26T20:10:53Z

Motivation and Context

Currently, the interactive sim viewer (example/viewer.py) lives in the examples/ directory is therefore not exposed within the habitat_sim python module. As such, it cannot be used with Habitat-sim conda build or imported and extended in Habitat-lab.

This PR copies the viewer code into the src_python directory and makes minor edits for code style and flaking requirements.

See Habitat-lab PR 1099 for example use as external module to extend.

TODO:
Once this is ready for merge we'll update documentation and possibly deprecate the examples/viewer.py.

How Has This Been Tested

With Habitat-sim module installed,

run the new viewer from its python file locally

python src_python/habitat_sim/viewer/viewer.py --dataset data/scene_datasets/mp3d_example/mp3d.scene_dataset_config.json --scene 17DRP5sb8fy

run the new viewer from the module:

python -m habitat_sim.viewer.viewer --dataset data/scene_datasets/mp3d_example/mp3d.scene_dataset_config.json --scene 17DRP5sb8fy

Issue: module run is currently failing with:

Platform::WindowlessEglContext: cannot make context current: EGL_BAD_ACCESS
Platform::WindowlessEglApplication::makeCurrent(): cannot make context current: EGL_BAD_ACCESS
Failed to make platform current

Likely related: we require the following to avoid this issue with the python file run:

flags = sys.getdlopenflags()
sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)

possibly the module run is picking things up differently.

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

aclegg3 · 2023-01-26T20:17:22Z

@mosra thoughts on the EGL issue with running from the module?

aclegg3 · 2023-01-26T20:19:38Z

src_python/habitat_sim/viewer/viewer.py

+        self.display_font = text.FontManager().load_and_instantiate("TrueTypeFont")
+        self.display_font.open_file(
+            os.path.join(os.path.dirname(__file__), self.sim_settings["font_path"]),
+            13,
+        )


@mosra had to replace the relative filepath with a configurable one which can be updated in consumer code. Generally we can't expect a data folder to be available from conda build, so we should consider how to handle this.

Maybe we should include the font file with the python module source code instead?

Maybe we should include the font file with the python module source code instead?

I think that this would make sense.

Yep, that makes sense -- when John did that originally we weren't sure what the use case would be, and hardcoding the relative path to data/ was the most straightforward way.

jaraujo98 · 2023-01-27T08:50:51Z

I'm a strong supporter of this change! A couple of thoughts (from when I was thinking about this a few months ago):

Several of the current keybindings make sense for an example viewer, but might not be interesting for every viewer application, and it is helpful to have those keys free for different functionality. From that perspective, I think it could make sense to keep keybindings to a minimum in the base viewer class and add the remaining ones in an ExampleViewer subclass implemented in the examples/viewer.py file.
The same thing goes for the command line arguments. This makes sense for the example viewer, but maybe not for a base class definition inside src_python.

aclegg3 · 2023-01-27T18:00:18Z

keep keybindings to a minimum in the base viewer class and add the remaining ones in an ExampleViewer subclass implemented in the examples/viewer.py file... The same thing goes for the command line arguments...

Thanks, I agree. I'll start a design doc to help plan which UI features should be core vs. example. 👍

mosra · 2023-02-02T19:23:44Z

sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)

A bit of background, just to have the full picture here -- by default, Python loads every module in its own namespace (RTLD_LOCAL), making it impossible for a different module to see its globally exported symbols. This makes sense for security reasons. RTLD_GLOBAL disables that. I'm not sure why the behavior differs between python -m module and python module.py, but it's probably documented somewhere.

The other half of the problem that Habitat uses Magnum compiled statically, and while I eliminated most mutable globals, Magnum has still three left -- for the plugin manager, for logging redirection and for the GL context. Those, with Magnum being compiled statically, get one copy in the habitat_sim.so Python module and one in the magnum.so Python module, and then I have to do cursed things to make both modules refer to the same globals so operation done in one module is visible by the other. And those cursed things then only work with RTLD_GLOBAL. Without that, being each loaded in its own namespace, the modules would need to share a common address of the globals in some manual way, possibly by going through Python, and so far I didn't figure out a nice way to do this that wouldn't become a new problem on its own.

A real solution to all these problems would be to have Magnum compiled dynamically, but that was historically not an option as far as I remember. Maybe we could reconsider, it'd definitely fix 98 of 100 recurring nightmares I have :D

The EGL error is most probably related to the above -- EGL_BAD_MATCH is emitted if the context is already active in another thread, so it's possible that due to the global GL context not being shared properly between the two modules it was thinking the context wasn't active in another thread (while it actually was, for example in the background renderer if that's enabled in the build) and tried to use it.

TL;DR: you'll still need the flag there, at least until Magnum gets compiled dynamically, or a different solution is discovered.

copy viewer.py into habitat_sim module for external exposure

585b96b

aclegg3 requested review from mosra, eundersander and 0mdc January 26, 2023 20:10

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 26, 2023

aclegg3 mentioned this pull request Jan 26, 2023

[WIP] Lab interactive viewer extending sim viewer facebookresearch/habitat-lab#1099

Draft

7 tasks

aclegg3 commented Jan 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - Add viewer.py to Habitat-sim module #1990

[WIP] - Add viewer.py to Habitat-sim module #1990

aclegg3 commented Jan 26, 2023 •

edited

Loading

aclegg3 commented Jan 26, 2023 •

edited

Loading

aclegg3 Jan 26, 2023

0mdc Jan 27, 2023

mosra Feb 2, 2023

jaraujo98 commented Jan 27, 2023

aclegg3 commented Jan 27, 2023

mosra commented Feb 2, 2023 •

edited

Loading

[WIP] - Add viewer.py to Habitat-sim module #1990

Are you sure you want to change the base?

[WIP] - Add viewer.py to Habitat-sim module #1990

Conversation

aclegg3 commented Jan 26, 2023 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

aclegg3 commented Jan 26, 2023 • edited Loading

aclegg3 Jan 26, 2023

Choose a reason for hiding this comment

0mdc Jan 27, 2023

Choose a reason for hiding this comment

mosra Feb 2, 2023

Choose a reason for hiding this comment

jaraujo98 commented Jan 27, 2023

aclegg3 commented Jan 27, 2023

mosra commented Feb 2, 2023 • edited Loading

aclegg3 commented Jan 26, 2023 •

edited

Loading

aclegg3 commented Jan 26, 2023 •

edited

Loading

mosra commented Feb 2, 2023 •

edited

Loading